sock_dtls: move common code into sock_dtls_establish_session() #19142

benpicco · 2023-01-13T17:18:03Z

Contribution description

Currently the logic to establish a DTLS session is open-coded in each protocol that uses DTLS (CoAPS, DoDTLS).

To avoid code duplication, move the common code to establish a connection into a sock_dtls_connect() helper function that can be shared by it's users.

Testing procedure

tests/nanocoap_cli still works as before, can connect to examples/gcoap_dtls server via CoAPS
tests/gnrc_sock_dodtls ~~still untested~~

output of gnrc_sock_dodtls

> dodtls server [2001:db8::1]:853 5853 Client_identity secretPSK
DNS server: [2001:db8::1%5]:853

> dodtls request example.org inet6
example.org resolves to 2606:2800:220:1:248:1893:25c8:1946

> dodtls request example.org inet
example.org resolves to 93.184.216.34

Issues/PRs references

sys/include/net/sock/util.h

riot-ci · 2023-01-31T21:13:36Z

Murdock results

✔️ PASSED

df4ef80 sock_dodtls: make use of sock_dtls_establish_session()

Success	Failures	Total	Runtime
6865	0	6865	13m:50s

Artifacts

Documentation preview

sys/net/application_layer/sock_dodtls/sock_dodtls.c

sys/net/sock/sock_util.c

sys/net/application_layer/sock_dodtls/sock_dodtls.c

kfessel

looks good

sys/net/application_layer/sock_dodtls/sock_dodtls.c

fabian18

Please Squash. I think it is looking good too.
I´ve got a problem with python when setting up dodtls, even though the instructions are pretty detailed. Since you´ve tested it, it should be good.

benpicco · 2023-02-19T22:10:49Z

bors merge

bors · 2023-02-20T01:41:35Z

Build succeeded:

kfessel · 2023-02-20T12:09:49Z

sys/net/application_layer/sock_dodtls/sock_dodtls.c

    _cred_type = creds->type;
    _cred_tag = creds->tag;
    _id = (uint16_t)(random_uint32() & 0xffff);
 exit:
-    memset(_dns_buf, 0, sizeof(_dns_buf));  /* flush-out unencrypted data */
    mutex_unlock(&_server_mutex);
    return (res > 0) ? 0 : res;


@benpicco: the credman_delete(creds_tag, creds_type); got removed from line 184 maybe we need that in l. 206 if the connection could not be established since in that case it would not be closed -> that credman slot would be taken until reboot.

Kind of what I thought here.

missed that
but somehow i am still not fully convinced maybe the credman_delete in
_close_session is also wrong

maybe that whole file should never call any credman function (but find or get) since the add should be done in another part of RIOT

credman is weird - it's true that the credentials are only stored for the lifetime of the session.
Since they are added in _connect_server() they should also be removed if _connect_server() fails - unless they were already added to credman before.

Same goes for _close_session().

Now what I don't get is why credman is needed at all.

seems more like it is not used as intended

intention:
init:
add all your creeds to credman

connect:
use a cred by tag

done here:

connect
add this creed to credman
use the cred just added

but there where applications before that that did the credential handling themself and these are now add cred to credman
and that use the cred

if we use it as intended we would just have one place where all the creds are managed this would make the review of the creds on a device simpler)

seems like the move to cred man is just incomplete

#11564 (comment)

there is the reasoning why credman and not just a pointer to credentials

benpicco requested review from miri64, haukepetersen, PeterKietzmann, cgundogan and maribu as code owners January 13, 2023 17:18

github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Jan 13, 2023

benpicco requested a review from pokgak January 13, 2023 17:18

benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Jan 13, 2023

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 31, 2023

benpicco force-pushed the sock_dtls_connect branch from 394a32f to a41ecb4 Compare January 31, 2023 19:32

benpicco requested a review from kfessel January 31, 2023 19:34

miri64 reviewed Jan 31, 2023

View reviewed changes

sys/include/net/sock/util.h Outdated Show resolved Hide resolved

benpicco added the State: waiting for maintainer State: Action by a maintainer is required label Feb 2, 2023

benpicco changed the title ~~sock_dtls: move common code into sock_dtls_connect()~~ sock_dtls: move common code into sock_dtls_establish_session() Feb 8, 2023

benpicco added 2 commits February 8, 2023 15:57

sys/net/sock_util: add sock_dtls_establish_session()

4676472

nanocoap_sock: make use of sock_dtls_establish_session()

5134b5c

benpicco force-pushed the sock_dtls_connect branch from a76548a to a4f142d Compare February 8, 2023 14:58

benpicco requested a review from fabian18 February 10, 2023 15:18

fabian18 reviewed Feb 12, 2023

View reviewed changes

sys/net/application_layer/sock_dodtls/sock_dodtls.c Show resolved Hide resolved

sys/net/application_layer/sock_dodtls/sock_dodtls.c Show resolved Hide resolved

sys/net/sock/sock_util.c Show resolved Hide resolved

fabian18 reviewed Feb 12, 2023

View reviewed changes

sys/net/application_layer/sock_dodtls/sock_dodtls.c Show resolved Hide resolved

kfessel reviewed Feb 17, 2023

View reviewed changes

fabian18 reviewed Feb 17, 2023

View reviewed changes

sys/net/application_layer/sock_dodtls/sock_dodtls.c Outdated Show resolved Hide resolved

benpicco removed the State: waiting for maintainer State: Action by a maintainer is required label Feb 18, 2023

fabian18 approved these changes Feb 19, 2023

View reviewed changes

sock_dodtls: make use of sock_dtls_establish_session()

df4ef80

benpicco force-pushed the sock_dtls_connect branch from 2acc48a to df4ef80 Compare February 19, 2023 14:11

bors bot merged commit 96a7d0d into RIOT-OS:master Feb 20, 2023

benpicco deleted the sock_dtls_connect branch February 20, 2023 04:26

kfessel reviewed Feb 20, 2023

View reviewed changes

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sock_dtls: move common code into sock_dtls_establish_session() #19142

sock_dtls: move common code into sock_dtls_establish_session() #19142

benpicco commented Jan 13, 2023 •

edited

Loading

riot-ci commented Jan 31, 2023 •

edited

Loading

kfessel left a comment

fabian18 left a comment

benpicco commented Feb 19, 2023

bors bot commented Feb 20, 2023

kfessel Feb 20, 2023

fabian18 Feb 20, 2023

kfessel Feb 20, 2023

kfessel Feb 20, 2023

benpicco Feb 20, 2023

kfessel Feb 20, 2023 •

edited

Loading

kfessel Feb 20, 2023

sock_dtls: move common code into sock_dtls_establish_session() #19142

sock_dtls: move common code into sock_dtls_establish_session() #19142

Conversation

benpicco commented Jan 13, 2023 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Jan 31, 2023 • edited Loading

Murdock results

Artifacts

kfessel left a comment

Choose a reason for hiding this comment

fabian18 left a comment

Choose a reason for hiding this comment

benpicco commented Feb 19, 2023

bors bot commented Feb 20, 2023

kfessel Feb 20, 2023

Choose a reason for hiding this comment

fabian18 Feb 20, 2023

Choose a reason for hiding this comment

kfessel Feb 20, 2023

Choose a reason for hiding this comment

kfessel Feb 20, 2023

Choose a reason for hiding this comment

benpicco Feb 20, 2023

Choose a reason for hiding this comment

kfessel Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

kfessel Feb 20, 2023

Choose a reason for hiding this comment

benpicco commented Jan 13, 2023 •

edited

Loading

riot-ci commented Jan 31, 2023 •

edited

Loading

kfessel Feb 20, 2023 •

edited

Loading